Add ProxyJump support for SSH connections#1858
Add ProxyJump support for SSH connections#1858Bnjoroge1 wants to merge 9 commits intogeneralaction:mainfrom
Conversation
Greptile SummaryThis PR adds first-class
Confidence Score: 3/5Not safe to merge as-is; the proxy subprocess leaks on every failed test-connection attempt. One P1 resource leak (child process not destroyed on SSH client error) keeps the score below the P1 ceiling of 4. The rest of the implementation is well-structured with good test coverage. src/main/core/ssh/controller.ts — missing
|
| Filename | Overview |
|---|---|
| src/main/core/ssh/controller.ts | P1: proxySock child process leaks when client.on('error') fires — proxySock?.destroy() is missing from that handler, unlike the synchronous try/catch path which cleans up correctly. |
| src/main/core/ssh/proxy-jump-sock.ts | New file: ProcessSocket Duplex wrapper + buildProxyJumpSocket. Solid backpressure handling; two minor issues: multi-hop ProxyJump silently drops hops beyond the first, and the exit-code error message can read "exit code null" when both code and signal are null. |
| src/main/core/ssh/build-connect-config.ts | Refactored to resolve SSH config host aliases and attach ProxyJump socket after auth config is built; auth failure before socket creation means no resource leak. |
| src/main/core/ssh/sshConfigParser.ts | Adds multi-alias Host parsing, resolveSshConfigHost function, and ProxyJump directive parsing. Logic is clean; ProxyJump none is correctly treated as a no-op. |
| src/main/core/ssh/utils.ts | resolveIdentityAgent now delegates to resolveSshConfigHost; duplicate implementation removed cleanly. |
| src/shared/ssh.ts | Adds proxyJump?: string field to SshConfigHost interface. |
Comments Outside Diff (1)
-
src/main/core/ssh/controller.ts, line 127-130 (link)ProxyJump socket leaks on SSH client error
When
client.on('error')fires (e.g., authentication failure, timeout, connection refused),proxySockis resolved away without being explicitly destroyed. The underlyingssh -W ...child process is left running indefinitely until the parent process exits. Thetry/catchpath already callsproxySock?.destroy(), but the asyncclient.on('error')callback has no equivalent cleanup.Prompt To Fix With AI
This is a comment left during a code review. Path: src/main/core/ssh/controller.ts Line: 127-130 Comment: **ProxyJump socket leaks on SSH client error** When `client.on('error')` fires (e.g., authentication failure, timeout, connection refused), `proxySock` is resolved away without being explicitly destroyed. The underlying `ssh -W ...` child process is left running indefinitely until the parent process exits. The `try/catch` path already calls `proxySock?.destroy()`, but the async `client.on('error')` callback has no equivalent cleanup. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/main/core/ssh/controller.ts:127-130
**ProxyJump socket leaks on SSH client error**
When `client.on('error')` fires (e.g., authentication failure, timeout, connection refused), `proxySock` is resolved away without being explicitly destroyed. The underlying `ssh -W ...` child process is left running indefinitely until the parent process exits. The `try/catch` path already calls `proxySock?.destroy()`, but the async `client.on('error')` callback has no equivalent cleanup.
### Issue 2 of 3
src/main/core/ssh/proxy-jump-sock.ts:61
**Multi-hop ProxyJump chain silently truncated**
`ProxyJump` values can be comma-separated to define a multi-hop chain (e.g., `ProxyJump hop1,hop2`). `splitProxyJumpEntry` only takes the first hop — additional hops are silently discarded. Consider using `-J hop1,hop2` when there are multiple entries so the full chain is honoured.
```suggestion
// NOTE: only the first hop is forwarded to the local ssh binary via -W.
// Multi-hop ProxyJump (comma-separated) would require passing -J hop1,hop2
// instead; this is a known limitation of the current implementation.
const firstHop = proxyJump.split(',')[0]?.trim() ?? '';
```
### Issue 3 of 3
src/main/core/ssh/proxy-jump-sock.ts:139-141
**Ambiguous error message when process exits with neither a code nor a signal**
On some platforms a child process can exit with `code === null` and `signal === null` simultaneously. The current guard skips only `code === 0`, so `null` falls through and the message reads `"ProxyJump command failed (exit code null)"`, which is confusing.
```suggestion
child.once('exit', (code, signal) => {
if (sock.destroyed || code === 0) return;
const reason = signal ? `signal ${signal}` : code != null ? `exit code ${code}` : 'unknown exit';
```
Reviews (1): Last reviewed commit: "ssh: keep proxy jump host destination in..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| function splitProxyJumpEntry(proxyJump: string): { destination: string; port?: string } { | ||
| const firstHop = proxyJump.split(',')[0]?.trim() ?? ''; |
There was a problem hiding this comment.
ProxyJump values can be comma-separated to define a multi-hop chain (e.g., ProxyJump hop1,hop2). splitProxyJumpEntry only takes the first hop — additional hops are silently discarded. Consider using -J hop1,hop2 when there are multiple entries so the full chain is honoured.
| const firstHop = proxyJump.split(',')[0]?.trim() ?? ''; | |
| // NOTE: only the first hop is forwarded to the local ssh binary via -W. | |
| // Multi-hop ProxyJump (comma-separated) would require passing -J hop1,hop2 | |
| // instead; this is a known limitation of the current implementation. | |
| const firstHop = proxyJump.split(',')[0]?.trim() ?? ''; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/core/ssh/proxy-jump-sock.ts
Line: 61
Comment:
**Multi-hop ProxyJump chain silently truncated**
`ProxyJump` values can be comma-separated to define a multi-hop chain (e.g., `ProxyJump hop1,hop2`). `splitProxyJumpEntry` only takes the first hop — additional hops are silently discarded. Consider using `-J hop1,hop2` when there are multiple entries so the full chain is honoured.
```suggestion
// NOTE: only the first hop is forwarded to the local ssh binary via -W.
// Multi-hop ProxyJump (comma-separated) would require passing -J hop1,hop2
// instead; this is a known limitation of the current implementation.
const firstHop = proxyJump.split(',')[0]?.trim() ?? '';
```
How can I resolve this? If you propose a fix, please make it concise.| child.once('exit', (code, signal) => { | ||
| if (sock.destroyed || code === 0) return; | ||
| const reason = signal ? `signal ${signal}` : `exit code ${code}`; |
There was a problem hiding this comment.
On some platforms a child process can exit with code === null and signal === null simultaneously. The current guard skips only code === 0, so null falls through and the message reads "ProxyJump command failed (exit code null)", which is confusing.
| child.once('exit', (code, signal) => { | |
| if (sock.destroyed || code === 0) return; | |
| const reason = signal ? `signal ${signal}` : `exit code ${code}`; | |
| child.once('exit', (code, signal) => { | |
| if (sock.destroyed || code === 0) return; | |
| const reason = signal ? `signal ${signal}` : code != null ? `exit code ${code}` : 'unknown exit'; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/core/ssh/proxy-jump-sock.ts
Line: 139-141
Comment:
**Ambiguous error message when process exits with neither a code nor a signal**
On some platforms a child process can exit with `code === null` and `signal === null` simultaneously. The current guard skips only `code === 0`, so `null` falls through and the message reads `"ProxyJump command failed (exit code null)"`, which is confusing.
```suggestion
child.once('exit', (code, signal) => {
if (sock.destroyed || code === 0) return;
const reason = signal ? `signal ${signal}` : code != null ? `exit code ${code}` : 'unknown exit';
```
How can I resolve this? If you propose a fix, please make it concise.
|
Will address Greptile's comments. I was planning on leaving multi-hop impl for a different PR since my main use cases was a single hop but I'll add that. |
|
Hi @Bnjoroge1, |
Bnjoroge1
force-pushed
the
add-jumphost-eo743
branch
from
aabefba to
bbe7536
Compare
|
nit:
|
2 participants
Implements first-class ProxyJump support for SSH connections.
Refs #1857